feat: New parameter and cli arguments handling #1060

bjsowa · 2025-08-08T23:35:20Z

Let's be honest here. The way parameters are passed down from rosbridge_server to the Rosbridge protocol and internal classes is a complete mess:

Rosbridge library retrieves some parameters either from:
1. Protocol.parameters dictionary.
2. Node handle parameters.
3. Class variables overwritten by rosbridge_server.
RosbridgeWebsocketNode parameter handling is very complex. It's very hard to tell which parameters relate to the server and which are passed to the protocol.
The cli argument parsing contains a lot of repeated boilerplate code.
Some internal modules like publishers.py or message_conversion.py implement a singleton interface that expect parameters to be passed to them but they're either not passed or passed in a non-standard way.

This PR contains the following modifications:

Protocol class now allows passing the parameters through the constructor instead of class variable
All of the Capability classes retrieve their parameters through protocol.parameters upon initialization (No need to set topics_glob class variables) and use instance variables to get them later (e.g. self.topics_glob instead of Advertise.topics_glob).
The default values are defined in class level and are only overwritten in the instance if protocol.parameters dictionary contains the keys.
The ROS parameter and cli argument handling in RosbridgeWebsocketNode has been reworked:
- Added new module constants SERVER_PARAMETERS and PROTOCOL_PARAMETERS that contain parameter name, type, default value and description.
- Used argparse for parsing cli arguments (less boilerplate code and more user-friendly interface)
- Made ROS parameters take precedence over cli args (passing cli args modifies the default values for ROS parameters)
- A single class variable protocol_parameters is set in RosbridgeWebSocket handler, no need to create it on every opened connection
- Added missing actions_glob parameter
The publishers.py and message_conversion.py modules are now configured upon first initialization of the Protocol class.

Some changes in behavior:

message_conversion.py module needs to be configured before use. This is reflected in the test_message_conversion.py test.
The binary_encoder_type and bson_only_mode are now actually passed to message_conversion module and have effect.
Setting Node parameters before opening Protocol won't affect protocol parameters as the rosbridge_library does not use node_handle to retrieve parameters. All of parameters are passed in the dictionary in the Protocol constructor. This is reflected in tests.
(Opinionated) max_message_size actually has effect now, that is, even when client sets the fragment_size, the actual fragment size is capped to max_message_size value.

~~What's left to do:~~

~~I need to figure out how the bson parameters are handled, especially the relation between binary_encoder and bson_only_mode.~~
~~Some real life application testing~~

~~Even though this is still WIP, I would appreciate a review~~

Don't retrieve ROS parameters in rosbridge_library, instead, use parameters dictionary passed to RosbridgeProtocol for everything

Instead of passing them using a class variable

Respect max_message_size parameter when determining fragment size, even when client provides fragment_size parameter

sea-bass · 2025-08-09T13:32:02Z

I won't get to review this for a bit, but have you considered possibly using https://github.com/PickNikRobotics/generate_parameter_library?

Not sure how well this will do with hybrid ROS params and CLI arguments though, but at least may package up your params in a neat struct.

bjsowa · 2025-08-09T14:48:41Z

I won't get to review this for a bit, but have you considered possibly using https://github.com/PickNikRobotics/generate_parameter_library?

Not sure how well this will do with hybrid ROS params and CLI arguments though, but at least may package up your params in a neat struct.

Might be a good idea. I'll try to figure out whether I can use it together with cli args

bjsowa · 2025-08-09T15:36:16Z

@sea-bass I looked at the possibility of using generate_parameter_library and IMO it's not worth it. It will only add complexity instead of simplifying it and won't provide any useful features. All of the parameters are read-only (read only once at the start) and don't require any validation. Also it's not possible to retrieve parameter descriptions for cli args before declaring ROS parameters.

bjsowa · 2025-10-05T00:55:07Z

@sea-bass @EzraBrooks @MatthijsBurgh I think it's ready now. Would appreciate a review.

sea-bass

It does look cleaner! Have a few comments throughout.

Also for what it's worth, generate_parameter_library is not just read only once at the beginning. It can definitely make parameters runtime tunable. But it does rely on more tools and code generation that I am convinced are not necessary here.

sea-bass · 2025-10-05T23:46:22Z

rosbridge_library/src/rosbridge_library/capability.py

        self.protocol = protocol

+        if self.parameter_names and self.protocol.parameters:
+            for param_name in self.parameter_names:


This and the next line could be a single list comprehension

Could you show what specific you have in mind?

for param_name in self.parameter_names if param_name in self.protocol.parameters: setattr(self, param_name, self.protocol.parameters[param_name])

... actually I just noticed another option

for param_name, param_value in self.parameter_names.items(): if param_name in self.protocol.parameters: setattr(self, param_name, param_value)

this isn't a major comment, i was just staring at python

Is this actually a thing? I haven't seen such syntax in python

I just tested:

for name in list1 if name in list2: print(name)

And it gave me syntax error

ugh no, I missed a thing. It would actually be this, which already makes it too hard to read

for param_name in [p for p in self.parameter_names if p in self.protocol.parameters]:

Nah, this starts to look ugly. I might go with your other proposition though

I think #1060 (comment) may still be worthwhile though

... actually I just noticed another option

for param_name, param_value in self.parameter_names.items(): if param_name in self.protocol.parameters: setattr(self, param_name, param_value)

this isn't a major comment, i was just staring at python

I cannot use items() on parameter_names as it is a tuple of strings, not a dictionary. This would have to iterate over parameters instead:

if self.parameter_names and self.protocol.parameters: for param_name, param_value in self.protocol.parameters.items(): if param_name in self.parameter_names: setattr(self, param_name, param_value)

sea-bass · 2025-10-05T23:49:27Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

+
+    if parameters is not None:
+        if "binary_encoder" in parameters:
+            binary_encoder_type = parameters["binary_encoder"]


Rather than globals (yuck, though they were here before), I would do something like

binary_encoder_type = parameters.get("binary_encoder", None)

(Of course the , None is not needed above, but putting there for other parameters that have different defaults

Although I see that those globals are used by the other free functions, so are they necessary? Hmmn, what if configure() simply returns the values it needs?

binary_encoder_type global is not used by any function so it can be removed

I removed binary_encoder_type, one less global. binary_encoder and bson_only_mode are used by the functions though.

I had an idea for a refactor which would add a MessageConverter class that would have the extract_values and populate_instance methods and would take the protocol parameters in the constructor. The Protocol class would create an instance of the converter and use it instead of calling the free functions. This would allow to configure the converter with different parameters per Protocol instance (allowing for example using BSON and JSON at the same time).

When I started refactoring it, it turned out it would require changes in many places and I decided to not do this to not overcomplicate this PR.

rosbridge_library/src/rosbridge_library/protocol.py

rosbridge_library/test/internal/test_message_conversion.py

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

rosbridge_library/src/rosbridge_library/protocol.py

rosbridge_library/src/rosbridge_library/rosbridge_protocol.py

rosbridge_server/src/rosbridge_server/websocket_handler.py

bjsowa added 8 commits August 9, 2025 00:40

Retrieve all parameters from protocol parameters

f74e997

Don't retrieve ROS parameters in rosbridge_library, instead, use parameters dictionary passed to RosbridgeProtocol for everything

Rewrite parameter handling in Rosbridge Server

4e4660b

Declare ROS parameters read-only

1cf4cb4

Fix setting parameters through non-ROS cli arguments

e1875c6

Fix parameter handling in send_action_goal

db8cdb2

Pass parameters to Protocol through constructor

10c3f4e

Instead of passing them using a class variable

Improve parameter handling in protocol

ff3b7b0

Respect max_message_size parameter when determining fragment size, even when client provides fragment_size parameter

Fix passing protocol parameters in tests

5553b13

bjsowa requested review from sea-bass and EzraBrooks August 9, 2025 13:29

Merge branch 'ros2' into feature/rosbridge-parameters-rewrite

876a5f1

bjsowa changed the title ~~feat: Rewrite parameter and cli arguments handling in Rosbridge server~~ feat: New parameter and cli arguments handling Sep 19, 2025

bjsowa added 6 commits September 19, 2025 11:46

Revert some formatting changes

d9638b8

Fix mypy errors

7a1c6d1

Configure message_conversion module at node start

94a00e6

Simplify retrieving parameter capabilities

fa9f35c

Configure internal modules on Protocol init

5875708

Don't configure publisher manager on Capability init

de3324d

bjsowa marked this pull request as ready for review October 4, 2025 11:41

bjsowa requested a review from MatthijsBurgh October 4, 2025 11:42

sea-bass reviewed Oct 5, 2025

View reviewed changes

Don't alias message_conversion module

aeecc65

sea-bass approved these changes Oct 6, 2025

View reviewed changes

bjsowa added 3 commits October 6, 2025 03:01

Simplify capability parameter retrieving

22558da

Simplify parameter retrieving in Protocol

870da6c

Remove binary_encoder_type global

5d538d9

bjsowa requested a review from sea-bass October 6, 2025 01:14

MatthijsBurgh requested changes Oct 6, 2025

View reviewed changes

bjsowa added 4 commits October 6, 2025 10:40

Fix typo

d9601eb

Use super to call base class constructor

d9340f3

Assert node handle is instance of Node class

f9599e0

Fix lint errors

7a4b2fd

bjsowa requested a review from MatthijsBurgh October 6, 2025 09:32

MatthijsBurgh approved these changes Oct 7, 2025

View reviewed changes

feat: New parameter and cli arguments handling #1060

Are you sure you want to change the base?

feat: New parameter and cli arguments handling #1060

Uh oh!

Conversation

bjsowa commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sea-bass commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjsowa commented Aug 9, 2025

Uh oh!

bjsowa commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjsowa commented Oct 5, 2025

Uh oh!

sea-bass left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sea-bass Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sea-bass Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjsowa Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjsowa Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjsowa commented Aug 8, 2025 •

edited

Loading

sea-bass commented Aug 9, 2025 •

edited

Loading

bjsowa commented Aug 9, 2025 •

edited

Loading

sea-bass Oct 6, 2025 •

edited

Loading

sea-bass Oct 6, 2025 •

edited

Loading

bjsowa Oct 6, 2025 •

edited

Loading

bjsowa Oct 6, 2025 •

edited

Loading